fix(auth): honor pairing links for signed-in browsers and allow self sign-out - #261
fix(auth): honor pairing links for signed-in browsers and allow self sign-out#261wizzoapp[bot] wants to merge 6 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c4ea76108
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
…sign-out A signed-in browser that opened a pairing link was redirected home without redeeming the credential, so minted links stayed unused. /pair now applies the credential, replaces that browser's session with the grant's scopes, and revokes the previous session after a successful redeem. Replacement is fail-closed: if the existing session cannot be read or revoked, or the replacement cookie cannot be constructed, the exchange fails, the original session stays usable, and no replacement cookie is installed. A session without access:write also had no way to sign itself out. Sign-out is now self-service on this device; pairing-link minting and other-session revoke stay behind access:write.
A signed-in browser that opened a pairing link auto-applied it, so a standard pairing link could silently strip administrative access. Desktop Sign out revoked the cookie then immediately signed back in from the unbounded local bootstrap credential. Require an explicit Apply this link click when a session already exists, warn when that grant drops access, and keep the token so Retry works. Hide Sign out on Desktop with a reason. Do not revoke bearer sessions when installing a browser cookie. Pairing replacement stays fail-closed: a failed displacement does not install a replacement cookie. Grok 4.6 (T3 Code / Grok harness)
Sign-out and pairing replacement revoked the database session and cookie, but already-open sockets kept the captured principal and scopes. Revocation now interrupts those sockets, and RPC authorization re-reads the live session so a draining connection cannot keep old permissions. Grok 4.6 (T3 Code / Grok harness)
Pairing replacement now treats live-socket interrupt as part of displacement. If signaling the previous cookie session's sockets fails, the replacement cookie is not installed and the original session stays usable. Grok 4.6 (T3 Code / Grok harness)
3258f4e to
9d0fb3a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d0fb3adf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The desktop app's primary connection is the main-process bearer. Applying a pairing cookie there would consume the one-time link and report success while the administrative session stayed in place. Show why instead, matching desktop Sign out. Grok 4.6 (T3 Code / Grok harness)
Fail-closed replacement created the new session first, so a later socket close or revoke failure spent the one-time link, left an unreachable session, and still told the UI that nothing changed. If revoke persisted and MCP cleanup then failed, the browser was signed out with no cookie. Keep creation first so the credential is the reservation against concurrent applies. On displacement failure, revoke the new session, keep the original cookie, and report that the link was used up. If revoke persisted and only cleanup failed, install the replacement cookie and log the cleanup failure. Pairing copy now matches whether the session was kept, the link was already used, or the browser was signed out. Grok 4.6 (T3 Code / Grok harness)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff527788c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function* (args) { | ||
| yield* annotateEnvironmentRequest(args.endpoint.name); | ||
| const session = yield* EnvironmentAuthenticatedPrincipal; | ||
| const revoked = yield* serverAuth.revokeSession(session.sessionId); |
There was a problem hiding this comment.
When MCP peer-token persistence fails after SessionStore.revoke has already succeeded, revokeSession fails here before the response expires the cookie. The UI therefore reports that sign-out failed even though the browser session and sockets are already revoked, and a retry cannot clean up the still-active MCP peer credentials because the revoked cookie now receives 401; make revocation/cleanup recoverable or handle the persisted-revocation outcome before returning the error.
AGENTS.md reference: AGENTS.md:L73-L73
Useful? React with 👍 / 👎.
| const revokeError = yield* input.serverAuth.revokeSession(input.sessionId).pipe( | ||
| Effect.as(null), |
There was a problem hiding this comment.
When two tabs sharing the same cookie apply two different valid pairing links concurrently, both requests capture the same previous session and create a replacement before either revokes it. The first revoke returns true, but the second returns false because the old session is already revoked; Effect.as(null) discards that result, so both requests report success and install competing cookies, consuming both links and leaving one replacement session active but potentially displaced from the cookie jar. Treat a false revoke as a lost replacement race or serialize replacement against the previous session.
Useful? React with 👍 / 👎.
A signed-in browser that opened a pairing link was sent home without redeeming it. Minted links stayed unused (
consumedAtstayed null), and a session withoutaccess:writehad no in-product way to re-pair or sign out.Rebased onto latest main so the diff no longer includes unrelated trailing deletions from the temp-dir teardown fix. HEAD 3650106.
Pairing links for an already-signed-in browser. Opening a same-origin one-time pairing URL is a deliberate act.
/pairnow keeps the credential instead of discarding it, and the apply surface requires an explicit Apply this link click beforecreateBrowserSessionreplaces the current session. If the incoming grant drops scopes or narrows data access, or if either grant cannot be compared, the confirmation says so first. After a successful redeem, the previous cookie session is revoked so leftover privileged scopes cannot linger in another tab. Bearer and DPoP sessions on the same request are left alone. Pairing a different device still omits the first cookie, so both sessions remain.Fail closed on replacement. Displacement is a security boundary, so this reverses an earlier non-blocking suggestion to swallow internal errors and continue. The replacement cookie is not installed and the handler does not report success unless the previous cookie session is revoked and its live sockets are signaled. If reading that session, interrupting its sockets, or revoking it fails, the request returns an error the UI surfaces as "Could not replace the existing session, nothing changed." The caller's original session stays intact and usable.
Desktop. The desktop app does not apply a pairing link against its local backend session, for the same reason it does not offer Sign out: the primary transport is the main-process bearer. Applying a cookie would consume the link and report success while administrative access stayed.
Downgrade is possible and intentional: the link the user applied is authoritative. The confirmation click is the safety; self sign-out is the way out if they need a different grant afterward.
Self sign-out.
POST /api/auth/session/sign-outrevokes the caller's own session withoutaccess:writeand expires the browser cookie. Settings → Connections shows Sign out for an authenticated browser session. The desktop app does not offer Sign out for its local backend: the unbounded desktop bootstrap credential would recreate an administrative session on the next load. Minting pairing links and revoking other clients stay behindaccess:write. Administrative self-revoke on the existing clients/revoke endpoint remains forbidden.Mobile. Mobile pairing is a saved-environment flow, not this cookie session. The mobile app has no self-sign-out for a saved environment yet.
Live sockets. Closing sockets on revocation is the product guarantee, not revalidation-only. Sign-out and pairing replacement both go through session revoke, which now wakes waiters for every live WebSocket bound to that session. Other tabs drop immediately, including tabs sitting on an event stream. RPC authorization also re-reads the live session so a draining connection cannot keep revoked access or previously captured scopes, including after a weaker pairing grant. Revalidation-only would have left an already-open stream signed in until its next RPC.
Grok 4.6 (T3 Code / Grok harness)